Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 2D-SWE and move wet/dry functionality from Trixi.jl #18

Merged
merged 53 commits into from
Mar 8, 2024

Conversation

patrickersing
Copy link
Contributor

@patrickersing patrickersing commented Jan 23, 2024

This PR introduces the 2D shallow water equations and the wet/dry functionality, that is to be removed from Trixi.jl in trixi-framework/Trixi.jl#1809. The two-layer SWE, that are also removed will be introduced in a separate PR.

One notable change in this PR is that I changed from using Trixi: Trixi => using Trixi. While moving more code to TrixiShallowWater.jl, I noticed that it is becoming to cumbersome and hard to read when we qualify every symbol from Trixi.jl, especially when using repetitive basic functionalities such as nnodes or eachelement.
With this change we now only need to qualify functions that will be extended by TrixiShallowWater.jl or when accessing functions that are not exported by Trixi.jl.

Besides that most of the code changes are just copied over from Trixi.jl and adapted to the new package.

The PR should only be merged after removing these features from Trixi.jl in trixi-framework/Trixi.jl#1809

Reminder: We will need to bump the compat bounds in the Project.toml for Trixi to 0.7 once it is released.

Copy link

codecov bot commented Feb 23, 2024

Codecov Report

Attention: Patch coverage is 99.18888% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 99.25%. Comparing base (7d236c4) to head (42cf9ee).

Files Patch % Lines
...c/callbacks_stage/positivity_shallow_water_dg1d.jl 95.12% 2 Missing ⚠️
...c/callbacks_stage/positivity_shallow_water_dg2d.jl 95.45% 2 Missing ⚠️
src/solvers/indicators.jl 94.73% 1 Missing ⚠️
src/solvers/indicators_1d.jl 97.82% 1 Missing ⚠️
src/solvers/indicators_2d.jl 97.87% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main      #18      +/-   ##
===========================================
- Coverage   100.00%   99.25%   -0.75%     
===========================================
  Files            4       32      +28     
  Lines           85      941     +856     
===========================================
+ Hits            85      934     +849     
- Misses           0        7       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@andrewwinters5000 andrewwinters5000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly just formatting or some text updates to the docstrings. I only left two questions regarding how the min_max_speed_chen_noelle dispatches within the new package.

Project.toml Show resolved Hide resolved
examples/tree_1d_dgsem/elixir_shallowwater_beach.jl Outdated Show resolved Hide resolved
src/TrixiShallowWater.jl Outdated Show resolved Hide resolved
test/test_tree_2d_shallowwater_wet_dry.jl Outdated Show resolved Hide resolved
src/solvers/indicators.jl Outdated Show resolved Hide resolved
src/solvers/indicators_1d.jl Outdated Show resolved Hide resolved
src/solvers/indicators_2d.jl Outdated Show resolved Hide resolved
src/equations/shallow_water_wet_dry_1d.jl Outdated Show resolved Hide resolved
src/equations/shallow_water_wet_dry_2d.jl Outdated Show resolved Hide resolved
@patrickersing patrickersing marked this pull request as ready for review February 29, 2024 10:58
Copy link
Member

@andrewwinters5000 andrewwinters5000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things are looking very nice! Just one small changes that I think is necessary.

src/equations/shallow_water_wet_dry_2d.jl Outdated Show resolved Hide resolved
Copy link
Member

@andrewwinters5000 andrewwinters5000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Fantastic work @patrickersing! I think we can merge this and continue onto adding in the two-layer equations. Do you want one last look @sloede ?

Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good to me and nearly ready to merge. I've just left a few suggestions, most of which are food for thought and could be addressed later.

Great job, @patrickersing, I am looking forward to seeing TrixiShallowWater.jl as a registered package :-)

.github/workflows/FormatCheck.yml Show resolved Hide resolved
src/TrixiShallowWater.jl Outdated Show resolved Hide resolved
src/equations/shallow_water_wet_dry_2d.jl Outdated Show resolved Hide resolved
src/equations/shallow_water_wet_dry_2d.jl Show resolved Hide resolved
src/equations/shallow_water_wet_dry_2d.jl Show resolved Hide resolved
Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great job, @patrickersing!

@patrickersing
Copy link
Contributor Author

Great, @andrewwinters5000 then we should be ready to merge! 🎉
Thanks a lot for the reviews!

Copy link
Member

@andrewwinters5000 andrewwinters5000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are good to go!

@andrewwinters5000 andrewwinters5000 merged commit c9b159c into trixi-framework:main Mar 8, 2024
6 of 9 checks passed
@patrickersing patrickersing deleted the swe_wetdry branch March 8, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants